-
Notifications
You must be signed in to change notification settings - Fork 32
WIP feat: allow running dev server from run commands #301
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
logger.info('Starting dev server...'); | ||
startDevServer({ | ||
root: projectRoot, | ||
reactNativePath: './node_modules/react-native', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm afraid this won't work in more complex setups, needs params (but it's temporary, right?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah
83e802f
to
5da0615
Compare
oops looke like the nx cache got in there |
92bc2df
to
e6a0b55
Compare
Yeah, not sure how to remove it. I don't see those changes in my commits |
|
fe01603
to
07e04e4
Compare
07e04e4
to
2da44ea
Compare
@OlimpiaZurek can you link some video recordings from this feature in action? Could you please describe your UX concerns from our sync? Thanks! |
Here’s the recording: recording.movWhen we run the android app and have metro open there, and at the same time we also run the iOS app — if I want to reload or open the dev tools for iOS, I switch over to the iOS tab. It can be a bit confusing since metro is running in the android tab. I don’t think it’s a big issue though. It depends on how each dev works. Most people probably reload or open the dev tools straight from the simulator anyway. |
root: projectRoot, | ||
reactNativePath, | ||
reactNativeVersion, | ||
platforms: { ios: {}, android: {} }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can get these from api.getPlatforms()
platforms: { ios: {}, android: {} }, | ||
args: { | ||
interactive: isInteractive(), | ||
clientLogs: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be something like args.clientLogs || true
.gitignore
Outdated
|
||
# Nx | ||
.nx |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
instead of adding nx to gitignore, use git rm --cached .nx
locally
if (startDevServer) { | ||
logger.info('Starting dev server...'); | ||
startDevServer({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On Android the dev server starts after binary is retrieved, and here it's before that. Running it here will cause the logs from both the bundler and the CLI to overwrite each other (you can see that on iOS example).
That's the tricky thing about this UX. We either need to:
- wait for all build/cache retrieval to happen and then run the dev server to avoid clashing logs
- start the dev server silently in the background and re-surface after the build is done / cache is retrieved – and we need to take into account that the bundler can be either Metro or Re.Pack (which is missing in this PR and should also expose the
start
field)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for pointing this out. I’ve changed it so the dev server starts after the build:
Screen.Recording.2025-10-13.at.13.06.45.mov
I’m focusing on the Metro part here. Should I also address Re.Pack in this PR, or do we want to handle that separately?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
adding it to Re.Pack would be lovely! especially that's it's pretty simple now (that's the reason I abstracted it this way)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we still have the logs interleaving each other:
Not the best DX. As a first iteration, I think it would be best to call metro server after the "success" log. Later on we could use grouping from @clack/prompts maybe together with Task Log feature, and move starting the server earlier. This would require further refactor of logging to be higher level, plus upgrading to latest v1-alpha, so it's better to tackle it separately
Summary
While running
run:android
orrun:ios
we should be able to start a dev server and keep it open after the build is doneTest plan